Skip to content

[2.0] Fix loopback in PhantomJS, fix karma tests - #344

Merged
bajtos merged 2 commits into
2.0from
fix-phantomjs-tests
Jun 26, 2014
Merged

[2.0] Fix loopback in PhantomJS, fix karma tests#344
bajtos merged 2 commits into
2.0from
fix-phantomjs-tests

Conversation

@bajtos

@bajtos bajtos commented Jun 23, 2014

Copy link
Copy Markdown
Member
  • Move configuration of Karma unit-tests from Gruntfile.js to a
    standalone file (test/karma.conf.js).
  • Add a new Grunt task karma:unit-ci to run Karma unit-tests in
    PhantomJS and produce karma-xunit.xml file that can be consumed
    by the CI server.
  • Add grunt karma:unit-ci to the test script in package.json.
  • Add es5-shim module to karma unit-tests in order to provide
    ES5-methods required by LoopBack.
  • Fix mixin(source) in lib/loopback.js to work in PhantomJS.
    Object.getOwnPropertyDescriptor() provided by es5-shim does not
    work in the same way as in Node.

Requires loopbackio/loopback-datasource-juggler#152

/to @ritch Please review.
/cc @rmg When this patch is landed, loopback is ready to run karma unit-tests on our Jenkins. npm test is already running karma via Grunt, is there anything else needed to enable karma tests on Jenkins?

@rmg

rmg commented Jun 23, 2014

Copy link
Copy Markdown
Member

@bajtos you can test what Jenkins will do by using sl-ci-run locally.

What kind of report is generated by the karma tests?

@bajtos

bajtos commented Jun 23, 2014

Copy link
Copy Markdown
Member Author

@rmg I am always forgetting about sl-ci-run. Ok, the Karma tests were run. The output is karma-xunit.xml in the project root dir.

Comment thread lib/loopback.js

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this breaking?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK it is not breaking. This check is preventing the following situation:

$ node
> Object.defineProperty({}, 'foo', undefined)
TypeError: Property description must be an object: undefined
    at Function.defineProperty (native)

desc is not defined when the key is not an own property of the source. MDN on getOwnPropertyDescriptor:

Return value
A property descriptor of the given property if it exists on the object, undefined otherwise.

What happens in PhantomJS:

  • for (var key in source) enumerates over bind function
  • the bind function is not an own property, getOwnPropertyDescriptor returns undefined
  • Object.defineProperty throws when the descriptor is not defined

@ritch

ritch commented Jun 23, 2014

Copy link
Copy Markdown
Member

@bajtos Are we including the es5 shim when not running the tests (eg. when built by browserify)?

@rmg

rmg commented Jun 23, 2014

Copy link
Copy Markdown
Member

karma-xunit.xml will be picked up by CI, but I have a feeling the xunit.xml file is no longer generated.

@rmg

rmg commented Jun 23, 2014

Copy link
Copy Markdown
Member

test please

@bajtos

bajtos commented Jun 24, 2014

Copy link
Copy Markdown
Member Author

@ritch Are we including the es5 shim when not running the tests (eg. when built by browserify)?

No. Most browsers support ES5 these days, with the exception of Internet Explorer 6-8.

yo angular is adding a conditional include to the generated index.html file:

    <!--[if lt IE 9]>
    <script src="bower_components/es5-shim/es5-shim.js"></script>
    <script src="bower_components/json3/lib/json3.min.js"></script>
    <![endif]-->

Even if this was not the case, including ES5 shim in the browserified bundle would not be a good idea, as it would unnecessarily increase the size of the browser bundle for all browsers.

I suppose this matter should be described in the documentation. The docs should mention that loopback depends on ES5 and users should load ES5-shim to run it in a browser that does not implement ES5.

/cc @crandmck

@bajtos

bajtos commented Jun 24, 2014

Copy link
Copy Markdown
Member Author

@rmg I have a feeling the xunit.xml file is no longer generated.

Yes, that's correct. I tried to change the test script to mocha && grunt karma:unit-ci, but it did not help. What do you advise?

@ritch

ritch commented Jun 24, 2014

Copy link
Copy Markdown
Member

Even if this was not the case, including ES5 shim in the browserified bundle would not be a good idea, as it would unnecessarily increase the size of the browser bundle for all browsers.

Sure.

I suppose this matter should be described in the documentation. The docs should mention that loopback depends on ES5 and users should load ES5-shim to run it in a browser that does not implement ES5.

We should have a guide for using loopback in browsers that includes this.

@bajtos bajtos added doc and removed doc labels Jun 24, 2014
@bajtos bajtos added this to the 2.0.0 milestone Jun 24, 2014
@rmg

rmg commented Jun 24, 2014

Copy link
Copy Markdown
Member

@bajtos the Gruntfile could have a top-level test task that runs tests based on the environment. It could check for $JENKINS_HOME and tell karma and mocha to generate xunit test reports (matching **/*xunit.xml) and produce a more developer friendly output when not under CI. I tried to avoid the need for that with the build framework our CI uses, but multiple test runners makes the automatic detection/replacement for overriding reporters more complicated.

@bajtos

bajtos commented Jun 24, 2014

Copy link
Copy Markdown
Member Author

@rmg that sounds plausible, I'll look into it tomorrow. thanks!

@bajtos

bajtos commented Jun 25, 2014

Copy link
Copy Markdown
Member Author

@rmg @ritch I have implemented grunt test task in the commit above, could you please review?

@rmg

rmg commented Jun 25, 2014

Copy link
Copy Markdown
Member

@bajtos it looks like I'll have to make changes to sl-ci-run so that it doesn't assume mocha and bypass npm test. If you could make mocha-and-karma the name of the task it should work without modifying sl-ci-run. In the future it might also be useful to use mocha-jenkins-reporter but that shouldn't be necessary right now.

@bajtos

bajtos commented Jun 26, 2014

Copy link
Copy Markdown
Member Author

@slnode test please

Miroslav Bajtoš added 2 commits June 26, 2014 13:30
- Move configuration of Karma unit-tests from `Gruntfile.js` to a
  standalone file (`test/karma.conf.js`).

- Add a new Grunt task `karma:unit-ci` to run Karma unit-tests in
  PhantomJS and produce karma-xunit.xml file that can be consumed
  by the CI server.

- Add grunt-mocha-test, configure it to run unit-tests.

- Add `grunt test` task that runs both karma and mocha tests,
  detects Jenkins to produce XML output on CI server.

- Modify the `test` script in `package.json` to run
  `grunt mocha-and-karma` (an alias for `grunt test`).
  The alias is required to trick `sl-ci-run` to run `npm test`
  instead of calling directly `mocha`.

- Add `es5-shim` module to karma unit-tests in order to provide
  ES5-methods required by LoopBack.

- Fix `mixin(source)` in lib/loopback.js to work in PhantomJS.
  `Object.getOwnPropertyDescriptor()` provided by `es5-shim` does not
  work in the same way as in Node.
@bajtos

bajtos commented Jun 26, 2014

Copy link
Copy Markdown
Member Author

grunt mocha-and-karma seems to work well.

bajtos added a commit that referenced this pull request Jun 26, 2014
[2.0] Fix loopback in PhantomJS, fix karma tests
@bajtos
bajtos merged commit dd5360a into 2.0 Jun 26, 2014
@bajtos
bajtos deleted the fix-phantomjs-tests branch June 26, 2014 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants